Skip to content

Introduce fetchpriority for Scripts and Script Modules #8815

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 15 commits into
base: trunk
Choose a base branch
from

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented May 19, 2025

  • Add new fetchpriority argument to go alongside the loading strategy (async & defer) when registering scripts via wp_register_script()/wp_enqueue_script(). This is a follow-up to the script loading strategies introduced in Core-12009.
  • Add a new 5th$args parameter to wp_register_script_module()/wp_enqueue_script_module() (and their corresponding methods) to mirror the same 5th $args parameter used in wp_register_script()/wp_enqeue_script(). This $args parameter same fetchpriority argument introduced for non-script modules. (Note that module scripts have a defer loading strategy by default, so adding support for a strategy would only be useful for async. This is not in the scope of this PR.) This will apply the fetchpriority attribute both to the SCRIPT[type="module"] tags as well as the LINK[rel="modulepreload"] tags for static import dependencies.
  • A new WP_Script_Modules::set_fetchpriority( string $id, string $priority ) method is added which allows the priority of registered scripts to be changed.
  • The default fetchpriority value for both scripts and script modules is auto.
  • When the fetchpriority attribute is emitted on SCRIPT and LINK tags, it is omitted if the value is auto since this is the default value.
  • The comment-reply script is given an explicit fetchpriority of low. While the script is already registered with the async loading strategy which Chrome causes the resource to be downloaded with a low priority by default, this is not the case for Safari or Firefox which use a default medium/normal priority. So the explicit low priority reduces the chance that the loading of the comment-reply script will interfere with the loading of resources needed in the critical rendering path (e.g. an LCP image element).
  • Script modules used for the view module scripts for blocks with Interactivity API support are given an explicit fetchpriority of low. The very first requirement/goal defined for the Interactivity API was for server-side rendering, therefore blocks should not depend on the view module script for initial rendering.
  • Tests have been written for the changes both to scripts and script modules, but the tests for the latter have additional changes since deficiencies were discovered (e.g. duplicate array keys, and global functions were untested).

Note

If Gutenberg is active, you won't see any changes without WordPress/gutenberg#70173 also checked out.

Trac ticket: https://core.trac.wordpress.org/ticket/61734


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@westonruter westonruter changed the title Use fetchpriority=low by default for script modules and add ability to override Introduce fetchpriority for Scripts and Script Modules May 20, 2025
@@ -175,11 +175,18 @@ function register_block_script_module_id( $metadata, $field_name, $index = 0 ) {
$block_version = isset( $metadata['version'] ) ? $metadata['version'] : false;
$module_version = isset( $module_asset['version'] ) ? $module_asset['version'] : $block_version;

$args = array();
if ( isset( $metadata['supports']['interactivity'] ) && $metadata['supports']['interactivity'] ) {
// TODO: Add ability for the fetchpriority to be specified in block.json for the viewScriptModule. In wp_default_script_modules() the fetchpriority defaults to low since server-side rendering is employed for core blocks, but there are no guarantees that this is the case for non-core blocks. That said, viewScriptModule entails Interactivity API, following the pattern from core it _should_ be SSR'ed and that is why this is the default for when the block supports interactivity.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe that property in block.json should become an object instead of a string. In the future we might need other properties in there too.

cc @gziolo

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely! See below 😄 #8815 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would make sense as a future enhancement, that this doesn't need to be implemented in this PR. If the low fetch priority is wrong, it could be fixed with a call to the new WP_Scripts_Modules::set_fetchpriority().

@@ -243,6 +250,7 @@ function register_block_script_handle( $metadata, $field_name, $index = 0 ) {
$script_args = array();
if ( 'viewScript' === $field_name && $script_uri ) {
$script_args['strategy'] = 'defer';
// TODO: There needs to be a way to specify that a script defined in a module is safe to use fetchpriority=low. Perhaps the viewScript should not only allow a handle string or a `file:./foo.js` string, but allow an an array of params like { "href": "./foo.js", "fetchpriority": "low" }. This would allow for more metadata to be supplied, like the script loading strategy. Related: <https://core.trac.wordpress.org/ticket/56408> and <https://core.trac.wordpress.org/ticket/54018>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes, exactly what I meant above 😅

Co-authored-by: Pascal Birchler <[email protected]>
Comment on lines 16 to 22
* @phpstan-type ScriptModule array{
* src: string,
* version: string|false|null,
* enqueue: bool,
* dependencies: array<array{ id: string, import: 'dynamic'|'static' }>,
* fetchpriority: 'auto'|'low'|'high',
* }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added these @phpstan annotations for myself as a safeguard while developing this patch. If others don't want PHPStan annotations in core, I'll remove prior to commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For better consistency, I think it's better to remove these annotations prior to commit indeed, as this is not something used elsewhere in Core. Also I dunno how the docblock parser will handle these info but I think it's going to add a lot of noise in dockblocks displayed on the generated documentation (same goes for other related phpstan annotations)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in 74a49e1

@westonruter westonruter marked this pull request as ready for review May 26, 2025 21:08
Copy link

github-actions bot commented May 26, 2025

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props westonruter, swissspidy, luisherranz, kraftbj, audrasjb.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this for regular scripts and it worked as advertised. I did confirm that add_data checks for is_scalar down the chain so lack of type checking here isn't an issue. The Web API, per MDN, will default to auto if not present or if the value is not valid (low, high, auto).

Copy link
Contributor

@audrasjb audrasjb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a comment about phpstan docblock annotations

Comment on lines 16 to 22
* @phpstan-type ScriptModule array{
* src: string,
* version: string|false|null,
* enqueue: bool,
* dependencies: array<array{ id: string, import: 'dynamic'|'static' }>,
* fetchpriority: 'auto'|'low'|'high',
* }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For better consistency, I think it's better to remove these annotations prior to commit indeed, as this is not something used elsewhere in Core. Also I dunno how the docblock parser will handle these info but I think it's going to add a lot of noise in dockblocks displayed on the generated documentation (same goes for other related phpstan annotations)

@audrasjb
Copy link
Contributor

@westonruter if this is still on track for 6.8.2, note that the PR is conflicting against trunk ;)

…into trac-61734

* 'trunk' of https://github.com/WordPress/wordpress-develop: (88 commits)
  Docs: Correct parameter and return types for some comment functions.
  Database: Reinstate test for MariaDB version in readme.
  Coding Standards: Remove unreachable `return` in `_get_block_template_file()`.
  Build/Test Tools: Fix playground workflow link in PR comment.
  Build/Test Tools: Revert [60324] while memory exhaustion issues in the GitHub Actions workflows are investigated.
  Docs: Remove redundant `@access` tags from DocBlocks in various files.
  Build/Test Tools: Bump the default baseline version used for performance comparison tests.
  Build/Test Tools: Update the currently supported branch that's used to determine the frequency of scheduled tests of old branches.
  Build/Test Tools: Update the upgrade testing matrix to reflect `6.8`.
  Build/Test Tools: Finalise changes to the WordPress versions used during upgrade testing from the development branch.
  Security: Update `composer/ca-bundle` to version 1.5.7.
  Site Health: Bump the recommended version of MariaDB to `10.6`.
  Build/Test Tools: Add inline docs to explain the PHPUnit strategy.
  do_blocks(): Free up transient memory leak
  Build/Test Tools: Switch to the GraphQL API for searching pull requests.
  Build/Test Tools: Use `issues` instead of `issuesAndPullRequests`.
  Database: Temporarily skip the `test_readme_mariadb_version` test method.
  Docs: Add missing DocBlock for the `_()` function in `compat.php`.
  Docs: Correct property type for `WP_Tax_Query::$no_results`.
  Coding Standards: Remove extra check in `wp_authenticate_application_password()`.
  ...
@westonruter
Copy link
Member Author

if this is still on track for 6.8.2, note that the PR is conflicting against trunk ;)

@audrasjb Merge conflicts resolved in 4197eb2

@westonruter westonruter requested review from azaozz and joemcgill June 21, 2025 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants